-
Notifications
You must be signed in to change notification settings - Fork 582
Improve some ListenetSet gep descriptions #3978
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rikatz The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
geps/gep-1713/index.md
Outdated
|
||
To attach to listeners in both a `Gateway` and `ListenerSet` the route MUST have two `parentRefs`: | ||
```yaml | ||
apiVersion: gateway.networking.k8s.io/v1 | ||
kind: HTTPRoute | ||
metadata: | ||
name: httproute-example | ||
spec: | ||
parentRefs: | ||
- name: some-workload-listeners | ||
- name: second-workload-listeners | ||
kind: ListenerSet | ||
sectionName: second | ||
- name: parent-gateway | ||
kind: Gateway | ||
sectionName: foo | ||
``` | ||
|
||
To attach to listeners in both a `Gateway` and `ListenerSet` the route MUST have two `parentRefs`: | ||
For instance, the following `HTTPRoute` attempts to attach to a listener defined in the parent `Gateway` using the sectionName `foo`. This is not valid and the route's status `Accepted` condition should be set to `False` | ||
|
||
```yaml | ||
apiVersion: gateway.networking.k8s.io/v1 | ||
kind: HTTPRoute | ||
metadata: | ||
name: httproute-example | ||
spec: | ||
parentRefs: | ||
- name: second-workload-listeners | ||
kind: ListenerSet | ||
sectionName: second | ||
- name: parent-gateway | ||
- name: some-workload-listeners |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess no real change here, just order, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, I wrote on my self review why I did the change. The example of the attachment of Gateway AND ListenerSet is more clear on this order, then we can go to the invalid examples :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make this invalid the kind
needs to be a ListenerSet
in the parentRef
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is still not clear to me, do you mean something like:
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
name: httproute-example
spec:
parentRefs:
- name: some-workload-listeners
kind: ListenerSet
sectionName: foo
?
Can you elaborate on this case with some example that would fail, I am not really understanding here. Thanks!
kind: Gateway | ||
sectionName: foo | ||
``` | ||
>--- Ricardo - above is a bit confusing, maybe ask Dave some clarification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @dprotaso
kind: Gateway | ||
sectionName: foo | ||
``` | ||
>--- Ricardo - above is a bit confusing, maybe ask Dave some clarification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dprotaso the example above is a bit confusing for me, probably because the API changed from when it was implemented.
I was wondering what it means for an HTTPRoute to try to attach a listener defined on a Gateway, using a section name foo
. If this section name exists on the Gateway it is valid, right? eg.:
listeners:
- name: foo
port: 80
protocol: HTTP
So how can the situation above be invalid? Maybe we can get the full yaml definition, with Gateway, ListenerSet and HTTPRoute for clarification here?
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One extra question I have on ListenerSet that is not clear for me, is the deduplication of listeners from different namespaces. Let me add an example: apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
name: parent-gateway
namespace: infra
spec:
allowedListeners:
namespaces:
from: All
---
apiVersion: gateway.networking.x-k8s.io/v1alpha1
kind: ListenerSet
metadata:
name: listener01
namespace: user01
spec:
parentRef:
name: parent-gateway
namespace: infra
kind: Gateway
group: gateway.networking.k8s.io
listeners:
- name: something
hostname: something.foo.com
protocol: HTTPS
port: 443
tls:
mode: Terminate
certificateRefs:
- kind: Secret
group: ""
name: cert
---
apiVersion: gateway.networking.x-k8s.io/v1alpha1
kind: ListenerSet
metadata:
name: listener01
namespace: user02
spec:
parentRef:
name: parent-gateway
namespace: infra
kind: Gateway
group: gateway.networking.k8s.io
listeners:
- name: something
hostname: something.foo.com
protocol: HTTPS
port: 443
tls:
mode: Terminate
certificateRefs:
- kind: Secret
group: ""
name: othercert Who owns |
on ListenerSet conflict management: at some point on this GEP there is a mention to it:
This is the statement for conflict management, but it is not really clear on what a conflict means. This way, during a discussion on gateway-api channel, we've reached the consensus of writing some conflict examples and how to deal with them, as part of the GEP, to make it easier on implementation and conformance. |
Thanks @rikatz. To bring the comments I had in the Slack thread here: I think there should be two rules:
Examples that illustrate those rules would be awesome. |
@youngnick added some examples and a whole section for conflict management, let me know wdyt :) |
@@ -115,6 +115,16 @@ type ListenerSetSpec struct { | |||
// 2. ListenerSet ordered by creation time (oldest first) | |||
// 3. ListenerSet ordered alphabetically by “{namespace}/{name}”. | |||
// | |||
// Regarding Conflict Management, Listeners in a ListenerSet follow the same |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add this new clause to the API types godoc?
Also we might want to call out sectionName behaves a bit differently where sibling ListenerSets can have the same sectionName.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, per Nick's suggestion I have used a similar approach of https://github.com/kubernetes-sigs/gateway-api/blob/main/apis/v1/gateway_types.go#L72 that contains the full description on godoc.
Conflicts are covered in the section 'ListenerConditions within a ListenerSet' | ||
Conflicts are covered in the section [Listener and ListenerSet conflicts](#listener-and-listenerset-conflicts) | ||
|
||
### Listener and ListenerSet conflicts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should combine this section into ListenerConditions within a ListenerSet
so that conflicts are covered in one section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO having a different section that defines what a conflict means from "what are the conditions" make it more clear for implementations.
We can probably refer on "ListenerConditions within a ListenerSet" back to the ### Listener and ListenerSet conflicts
to define what a conflict means, but I think that adding all on a single "Conditions" section would overload it and make it confusing.
management. | ||
|
||
With ListenerSet this validation should happen within the same ListenerSet resource, | ||
but MUST be validated also within a Gateway scope and all of the attached Listeners/ListenerSets. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this language needs to exclude sectionName when comparing it across sibling Listeners
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or refer to 'Optional Section Name'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry Dave, my brain is a bit fried today. Do you mean something like
"With ListenerSet this validation should happen within the same ListenerSet resource, but MUST be validated also within a Gateway scope and all of the attached Listeners/ListenerSets. The SectionName field is an exception for this validation, and while it should not conflict within the same ListenerSet, it can be duplicated between different ListenerSets" ?
protocol: HTTPS | ||
port: 443 | ||
conditions: | ||
- message: ListenerSet has conflicts with Gateway 'infra/parent-gateway' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we want to highlight the listener name in the error message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't wanted to leak cross namespace resource names
48fdd71
to
cb485fa
Compare
What type of PR is this?
/kind gep
What this PR does / why we need it:
This PR is a review of ListenerSet GEP, fixing some manifests and adding some comments to be further discussed
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: